feat: async recheck support by technicallyty · Pull Request #1062 · cosmos/evm

technicallyty · 2026-03-10T05:38:33Z

Description

we recently updated comet to no longer lock on recheck, pushing concurrency responsibility to the application.

changes:

route CheckTx through the app-side mempool insert worker instead of BaseApp.runTx
fix Cosmos rechecked tx tracking to replace entries by signer/nonce identity rather than pointer identity. the previous code was not allowing cosmos fee replacement txs

Closes: STACK-2455

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

tackled an existing issue or discussed with a team member
left instructions on how to review the changes
targeted the main branch

linear · 2026-03-10T05:38:36Z

STACK-2402 comet mempool is not releasing tx from guard

STACK-2455 EVM appside mempool should handle checktx locking

technicallyty · 2026-03-10T17:52:23Z

mempool/check_tx.go

+		// TODO: do we even do recheck anymore?
+		if request.Type == abci.CheckTxType_Recheck {
+			return &abci.ResponseCheckTx{Code: abci.CodeTypeOK}, nil
+		}


we can expect this to never be called with this mempool, yes? cc @mattac21

aljo242 · 2026-03-10T17:57:49Z

@greptile whats the scoop man

technicallyty · 2026-03-10T18:06:43Z

@greptile

vladjdk · 2026-03-10T18:07:43Z

@greptile hello

greptile-apps · 2026-03-10T18:13:32Z

Greptile Summary

This PR introduces async recheck support by routing all CheckTx calls through the app-side insert worker goroutine instead of BaseApp.runTx, and fixes Cosmos tx replacement in CosmosTxStore to match by signer/nonce identity rather than pointer identity. A new check-tx-timeout config field (default 30 s) bounds how long the handler blocks waiting for an insert result.

Key changes:

NewCheckTxHandler now decodes the raw tx bytes and calls mempool.Insert with a bounded context, returning the insert error directly to CometBFT
CosmosTxStore.AddTx grows a secondary keys map[string]int keyed on the canonical signer/nonce tuple so that fee-replacement txs correctly overwrite the old entry
GetCheckTxTimeout / EVMMempoolCheckTxTimeout flag / MempoolConfig.CheckTxTimeout wire the new configurable timeout end-to-end

Issue found:

The handler does not check request.Type before processing, so CheckTxType_Recheck requests are treated identically to new transactions. Since the tx is already in the pool, mempool.Insert returns ErrAlreadyKnown (non-OK), and CometBFT interprets a non-OK recheck response as the transaction becoming invalid — potentially evicting valid transactions silently. The included TestRecheckIsNoOp test will also fail for this reason (it sends malformed bytes expecting CodeTypeOK, but the decoder returns an error → non-OK response).

Confidence Score: 2/5

Not safe to merge — the missing recheck type guard will cause TestRecheckIsNoOp to fail in CI and can silently evict valid transactions from CometBFT's mempool during normal block processing.
The core logic change (routing CheckTx through the insert worker) is sound and the CosmosTxStore replacement fix is correct. However, the handler unconditionally processes CheckTxType_Recheck through the insert path — meaning every recheck for an already-known transaction will return ErrAlreadyKnown (non-OK), causing CometBFT to evict those transactions. This is a runtime correctness bug that would manifest during normal validator operation, not just in tests.
mempool/check_tx.go requires a CheckTxType_Recheck early-return guard before the decode and insert logic.

Important Files Changed

Filename	Overview
mempool/check_tx.go	New async handler routes all CheckTx requests — including Recheck — through the insert worker. Missing `CheckTxType_Recheck` guard will cause TestRecheckIsNoOp to fail and can silently evict valid transactions during recheck.
mempool/tx_store.go	Adds signer/nonce-keyed replacement logic to CosmosTxStore via a new `keys` map. Logic looks correct for single- and multi-signer txs; the store is rebuilt per block so bounded growth is not a concern.
server/server_app_options.go	GetCheckTxTimeout allows 0 to pass through (guards only against < 0), while NewCheckTxHandler panics on timeout <= 0. A user setting check-tx-timeout = "0s" passes GetCheckTxTimeout but panics the node at startup. (Note: previously flagged thread item.)
mempool/check_tx_test.go	New test suite covering EVM and Cosmos CheckTx paths. TestRecheckIsNoOp expects CodeTypeOK for a recheck with malformed bytes — this will fail without a Recheck type guard in the handler.
server/config/config.go	Adds CheckTxTimeout to MempoolConfig with a 30s default and Validate() enforcement (rejects <= 0). Also removes HistoricalGRPCAddressBlockRange propagation from SDK config — unrelated to this PR but a silent behaviour change.
evmd/mempool.go	Passes Trace() and GetCheckTxTimeout to NewCheckTxHandler. Wiring is correct; panic risk flows from the GetCheckTxTimeout/handler mismatch noted elsewhere.

Sequence Diagram

sequenceDiagram
    participant C as CometBFT
    participant H as CheckTxHandler
    participant D as TxDecoder
    participant W as InsertWorker
    participant MP as ExperimentalEVMMempool

    C->>H: RequestCheckTx (New or Recheck)
    Note over H: ⚠️ Type not checked — Recheck<br/>takes same path as New
    H->>D: TxDecoder(request.Tx)
    alt decode error
        D-->>H: error
        H-->>C: ResponseCheckTx (non-OK)
    else decode success
        D-->>H: sdk.Tx
        H->>H: context.WithTimeout(Background, timeout)
        H->>MP: Insert(ctx, tx)
        MP->>W: send insertRequest{tx, errC}
        W->>MP: process tx (validate, add to pool)
        W-->>MP: errC <- result
        MP-->>H: err (nil or ErrAlreadyKnown etc.)
        alt insert OK
            H-->>C: ResponseCheckTx{Code: OK}
        else insert error (e.g. ErrAlreadyKnown on recheck)
            H-->>C: ResponseCheckTx (non-OK)
            Note over C: Interprets non-OK recheck as<br/>invalid tx → evicts from mempool
        end
    end

_{Last reviewed commit: 4fe1466}

mempool/check_tx.go

mempool/tx_store.go

mattac21

could you also rebase this off of main? the tests are pretty broken on feat/krakatoa but if you go to main you should be able to get all of the system unit and integration tests passing

mattac21 · 2026-03-11T14:48:05Z

mempool/check_tx.go

+
+		ctx, cancel := context.WithTimeout(context.Background(), timeout)
+		defer cancel()
+		errC, err := mempool.insert(ctx, tx)


the mempool.Insert function already waits on the errC or ctx to be done like you are doing here. could we use that instead of using the private mempool.insert?

updated change to public method

mempool/check_tx.go

mattac21 · 2026-03-11T14:53:22Z

mempool/tx_store.go

 	index map[sdk.Tx]int
+	keys  map[string]int


keys is now serving the same purpose as index right? we can remove index now I think

mattac21 · 2026-03-11T14:58:16Z

mempool/tx_store.go

 	s.mu.Lock()
 	defer s.mu.Unlock()

+	if key, ok := cosmosTxKey(tx); ok {


hm thinking through this, im not actually sure this replacement is safe to do without rechecking all txs for this account, when we replace a tx we may want to just remove all txs > replaced nonce from the tx store for this account and then let them just be included in the next block.

I think this isn't safe because we would need to recheck all txs after the replaced one onto of the state of this new tx, which we are not doing. for example if we recheck txs 4 5 and 6 of an an account and include them in the tx store, then someone can replace tx 4 with a completely different tx that may have invalidated 5 and 6, but we are not rechecking those against the new tx 4's context, which may then cause the proposal to be invalid.

I think this is an issue for evm txs in the legacypool too that we need to address.

working on a separate PR to address this issue in both evm and cosmos tx stores

mattac21 · 2026-03-11T14:59:26Z

mempool/check_tx.go

+			select {
+			case err := <-errC:
 				if err != nil {
-					return sdkerrors.ResponseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, false), nil


were the anteEvents also always nil here? we are missing out on events now as well with this? should we modify the response of mempool.Insert to return some of this info?

i checked on v0.53.x and main, calling broadcast tx sync never returned anything other than code, tx hash, and a log if it failed.

you can see comet stripping down the response here: https://github.com/cometbft/cometbft/blob/1bb8b386fc366bc4655dede0535e16d1ad669c7d/rpc/core/mempool.go#L58-L64

got it, tested on gaia as well earlier and this was the case too

aljo242 · 2026-03-12T20:01:30Z

@greptile re-review

mempool/check_tx.go

server/config/config.go

technicallyty · 2026-03-12T21:53:44Z

@greptile re-review

greptile-apps · 2026-03-12T21:57:51Z

mempool/check_tx.go

+	return func(_ types.RunTx, request *abci.RequestCheckTx) (*abci.ResponseCheckTx, error) {
+		tx, err := mempool.txConfig.TxDecoder()(request.Tx)
 		if err != nil {
-			// detect if there is a nonce gap error (only returned for EVM transactions)
-			if errors.Is(err, ErrNonceGap) || errors.Is(err, ErrNonceLow) {
-				// send it to the mempool for further triage
-				err := mempool.InsertInvalidNonce(request.Tx)
-				if err != nil {
-					return sdkerrors.ResponseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, false), nil
-				}
-			}
-			// If its already known, this can mean the the tx was promoted from nonce gap to valid
-			// and by allowing ErrAlreadyKnown to be silent, we allow re-gossiping of such txs
-			// this also covers the case of re-submission of the same tx enforcing overpricing for replacement
-			if errors.Is(err, txpool.ErrAlreadyKnown) {
-				return sdkerrors.ResponseCheckTxWithEvents(nil, gInfo.GasWanted, gInfo.GasUsed, anteEvents, false), nil
-			}
-
-			// anything else, return regular error
-			return sdkerrors.ResponseCheckTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, false), nil
+			return sdkerrors.ResponseCheckTxWithEvents(err, 0, 0, nil, debug), nil
 		}

-		return &abci.ResponseCheckTx{
-			GasWanted: int64(gInfo.GasWanted), // #nosec G115 -- this is copied from the Cosmos SDK
-			GasUsed:   int64(gInfo.GasUsed),   // #nosec G115 -- this is copied from the Cosmos SDK
-			Log:       result.Log,
-			Data:      result.Data,
-			Events:    types.MarkEventsToIndex(result.Events, nil),
-		}, nil
+		ctx, cancel := context.WithTimeout(context.Background(), timeout)
+		defer cancel()
+		if err := mempool.Insert(ctx, tx); err != nil {
+			return sdkerrors.ResponseCheckTxWithEvents(err, 0, 0, nil, debug), nil
+		}
+		return &abci.ResponseCheckTx{Code: abci.CodeTypeOK}, nil
 	}


Recheck requests not short-circuited — breaks TestRecheckIsNoOp and risks tx eviction

The handler does not inspect request.Type, so abci.CheckTxType_Recheck requests take the same code path as new transactions.

Two concrete failure modes follow:

TestRecheckIsNoOp will fail. The test submits []byte("not-a-real-tx") with CheckTxType_Recheck and asserts CodeTypeOK. With the current code the decoder will return an error and sdkerrors.ResponseCheckTxWithEvents will produce a non-CodeTypeOK response.

Valid in-mempool transactions get evicted during recheck. When CometBFT fires a recheck for a tx that is already in the pool, mempool.Insert will return ErrAlreadyKnown (non-OK). CometBFT interprets a non-OK recheck response as the transaction having become invalid and removes it from its tracking — silently draining the mempool.

Since the app sets OperateExclusively = true, CometBFT delegates full mempool management to the application layer. Recheck is therefore a no-op from CometBFT's perspective and should always return CodeTypeOK without hitting the insert worker:

return func(_ types.RunTx, request *abci.RequestCheckTx) (*abci.ResponseCheckTx, error) { if request.Type == abci.CheckTxType_Recheck { return &abci.ResponseCheckTx{Code: abci.CodeTypeOK}, nil } tx, err := mempool.txConfig.TxDecoder()(request.Tx) ... }

codecov · 2026-03-12T22:34:07Z

Codecov Report

❌ Patch coverage is 69.23077% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.27%. Comparing base (bea1ab9) to head (9dd5c85).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
mempool/check_tx.go	53.84%	1 Missing and 5 partials ⚠️
server/server_app_options.go	61.53%	2 Missing and 3 partials ⚠️
mempool/tx_store.go	90.90%	1 Missing and 1 partial ⚠️
server/config/config.go	33.33%	2 Missing ⚠️
server/start.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1062      +/-   ##
==========================================
+ Coverage   64.11%   65.27%   +1.16%     
==========================================
  Files         331      331              
  Lines       23303    23302       -1     
==========================================
+ Hits        14941    15211     +270     
+ Misses       6946     6920      -26     
+ Partials     1416     1171     -245

Files with missing lines	Coverage Δ
server/flags/flags.go	`0.00% <ø> (ø)`
server/start.go	`0.00% <0.00%> (ø)`
mempool/tx_store.go	`88.67% <90.90%> (-11.33%)`	⬇️
server/config/config.go	`41.78% <33.33%> (-5.12%)`	⬇️
server/server_app_options.go	`41.74% <61.53%> (-13.81%)`	⬇️
mempool/check_tx.go	`36.36% <53.84%> (-52.53%)`	⬇️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

technicallyty changed the title ~~recheck support~~ feat: async recheck support Mar 10, 2026

technicallyty commented Mar 10, 2026

View reviewed changes

technicallyty requested review from Eric-Warehime, aljo242, mattac21 and vladjdk March 10, 2026 17:52

greptile-apps bot reviewed Mar 10, 2026

View reviewed changes

mempool/check_tx.go Outdated Show resolved Hide resolved

mempool/check_tx.go Outdated Show resolved Hide resolved

mempool/check_tx.go Show resolved Hide resolved

mempool/tx_store.go Show resolved Hide resolved

mattac21 reviewed Mar 11, 2026

View reviewed changes

technicallyty changed the base branch from feat/krakatoa to main March 11, 2026 17:17

technicallyty changed the base branch from main to feat/krakatoa March 11, 2026 17:17

mempool checktx handler updates

c54ef7b

technicallyty force-pushed the technicallyty/STACK-2402-krakatoa-recheck branch from f7e5c47 to c54ef7b Compare March 11, 2026 17:55

technicallyty changed the base branch from feat/krakatoa to main March 11, 2026 17:55

technicallyty added 2 commits March 11, 2026 10:57

Remove iavlx merge cflicts

2a8c3e8

no rechecktx

0ce99fc

greptile-apps bot reviewed Mar 12, 2026

View reviewed changes

mempool/check_tx.go Show resolved Hide resolved

mempool/check_tx.go Outdated Show resolved Hide resolved

server/config/config.go Outdated Show resolved Hide resolved

technicallyty added 3 commits March 12, 2026 14:51

fix comments referring to 0 timeout

0169dc5

fix pointless err resassignment

b26a6bd

checktxtimeout must be nonzero

4fe1466

greptile-apps bot reviewed Mar 12, 2026

View reviewed changes

panic on recheck

9dd5c85

Conversation

technicallyty commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Author Checklist

Uh oh!

linear bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aljo242 commented Mar 10, 2026

Uh oh!

technicallyty commented Mar 10, 2026

Uh oh!

vladjdk commented Mar 10, 2026

Uh oh!

greptile-apps bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattac21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technicallyty Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aljo242 commented Mar 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

technicallyty commented Mar 12, 2026

Uh oh!

greptile-apps bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 12, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

technicallyty commented Mar 10, 2026 •

edited

Loading

linear bot commented Mar 10, 2026 •

edited

Loading

greptile-apps bot commented Mar 10, 2026 •

edited

Loading

technicallyty Mar 11, 2026 •

edited

Loading